fix(cascade): restrict master from every restricted Part; validate part_integrity#1484
fix(cascade): restrict master from every restricted Part; validate part_integrity#1484dimitri-yatsenko wants to merge 2 commits into
Conversation
…rt_integrity Two fixes + one semantics pin from the 2.3 post-release audit: 1. part_integrity='cascade' under-restricted the master when two Parts of the same master were restricted via different FK paths from the seed: the upward part->master walk was deduplicated per MASTER (first Part wins), so later Parts never contributed their master rows. Empirically, with Ext <- Master.PartA and Ext <- Master.PartB (part rows under masters 1 and 2), cascading from Ext restricted only master 1 — the delete then removed master 2's PartB row while master 2 survived, silently violating the compositional integrity 'cascade' promises. The dedup is now keyed by (part, master) PAIR: every restricted Part fires the walk once. The pair set is finite and global across passes, so the multi-pass loop still terminates; duplicate master restrictions from sibling re-fires are harmless under OR semantics. 2. Eager part_integrity validation: Diagram.cascade() accepted any string (a typo like 'casade' silently degraded to enforce/ignore behavior) and Table.drop() only compared == 'enforce' (drop(part_integrity='cascade') silently behaved as 'ignore' although cascade is delete-only). Both now raise ValueError, mirroring Table.delete(). 3. Pins shipped trace semantics at master-part boundaries: trace walks ancestor edges only and does NOT descend from an ancestor Master into its Parts (merge shape Parent -> Master.P -> Master -> Child: trace reaches Master; Master.P and Parent raise). Matches provenance.md; corrects the unimplemented down-collection described on discussion 1232; capability tracked as #1481. Tests: two-parts-same-master regression (fails on the old dedup), cascade/drop validation, merge-shape trace pin. Full cascade+trace suites 46/46 on MySQL and PostgreSQL; wider regression (cascading_delete, dependencies, unit) 333 passed.
…erage gaps Locks the behaviors the post-2.3 audit verified, so regressions in either direction — a documented bypass silently closing, or a blocking path silently opening — become visible. test_strict_provenance_limits.py: len/bool and query-expression 'in' bypass (documented); CLASS-form 'in' RAISES (gated __iter__); restriction-by-table bypasses; join with undeclared operand RAISES (the one gated read path, previously untested); Aggregation __len__ bypasses while its fetch RAISES; delete_quick ungated (documented); INSERT...SELECT skips per-row key consistency (documented exception); self.upstream[SelfPart] raises while direct self.PartName read is allowed; context cleared after raising make(); nested push/pop token restore. test_cascade_integrity.py: enforce post-check raises 'before its master' AND rolls back; empty-materialization sentinel previews master at 0 and deletes cleanly; U3 upward arm with SECONDARY -> master FK restricts only the correct master (bare proj() would restrict both). Also corrects the provenance.py module docstring (audit item B6): context is pushed/popped inside _populate_one in whichever process/thread runs make() — the old across-threads/fork-inheritance rationale was inaccurate. Behavior unchanged. New files only — no conflicts with the open #1480/#1484 branches.
dimitri-yatsenko
added
the
bug
MilagrosMarin
left a comment
MilagrosMarin
left a comment
There was a problem hiding this comment.
Thanks @dimitri-yatsenko — real integrity gap caught. The (part, master) pair-keyed dedup is minimal and precisely-targeted, and the repro in test_cascade_two_parts_same_master_both_restrict (Ext ← {PartA, PartB} sharing one Ext row across two different masters) hits exactly the topology the old visited_masters set silently dropped. Under OR semantics the extra walks can only add, never subtract, so no risk of over-restricting cases the old behavior got right.
The eager validation on Diagram.cascade correctly mirrors Table.delete, and rejecting "cascade" on Table.drop is the right call — better a loud error than the silent "ignore" degradation.
The trace-stops-at-master pin is well-reasoned: it locks the pure-ancestor-walk semantics against a future well-intentioned "fix" toward the down-collection that discussion #1232 described but was never implemented.
One minor: the _propagate_restrictions docstring doesn't mention the pair-keyed invariant — future maintainers will see the inline comment on visited_part_master_pairs, but a one-liner at the method-docstring level ("each restricted Part fires its own upward walk once") would help someone landing here without the commit context.
Approving.
2 participants
Second code fix from the 2.3 post-release audit (companion to #1480).
1.
part_integrity="cascade"under-restriction — CRITICAL (empirically confirmed, both backends)The upward part→master walk was deduplicated per master (
visited_masters, first Part wins). When two Parts of the same master were restricted via different FK paths from the seed, the second Part never contributed its master rows:Ext ← Master.PartA,Ext ← Master.PartB; PartA row (ext 1) under master 1, PartB row (ext 1) under master 2.Diagram.cascade(Ext & {"ext_id": 1}, part_integrity="cascade")restricted only master 1 — the delete then removed master 2's PartB row while master 2 survived, silently violating the compositional integrity"cascade"promises. (The default"enforce"catches this topology via its post-check; only"cascade"failed silently.)Fix: dedup by (part, master) pair — every restricted Part fires the walk once. Termination is preserved (finite pair set, global across passes); sibling re-fires append duplicate master restrictions, harmless under OR semantics. Distinct from the documented single-shortest-path limitation.
2. Eager
part_integrityvalidationDiagram.cascade()accepted any string — a typo ("casade") silently degraded to enforce/ignore behavior. Now raisesValueError(mirrorsTable.delete).Table.drop()only compared== "enforce"—drop(part_integrity="cascade")silently behaved as"ignore"although cascade is delete-only. Now raisesValueError.3. Merge-shape trace semantics pinned
New test
test_trace_stops_at_master_no_part_down_collection: trace walks ancestor edges only and does not descend from an ancestor Master into its Parts (Parent → Master.P → Master → Child: trace reaches Master;Master.P/Parentraise). Matches provenance.md §2; corrects the unimplemented down-collection described in discussion #1232 (correction posted there); the capability is tracked as #1481. The pin exists so a future "fix" toward that promise can't silently change semantics.Validation
master count == 1), passes on the fix (== 2, and end-to-end delete removes both compositional units, control master untouched).Docs: the audit-batch PR (datajoint/datajoint-docs#195) adds the cascade Limitations section and does not document the once-per-master behavior — because this PR fixes it.